feat(worker): withhold stale PR review output when the head moves mid-session (#8) - #42
Merged
Merged
Conversation
…-session (#8) Re-fetch the PR refs after the session and before publication. If the head advanced while the review ran, complete the Check Run as neutral/Stale, surface 'Status: superseded' on the marker-backed status comment, and mark the task superseded in the Cave task list — the findings are never presented as if they covered the current head. The newer push's own event (or a maintainer retry) reviews the fresh head. This closes the last gap in issue #8: check runs already attach to resolved immutable SHAs (no 'HEAD' placeholders since the #10 target-resolution work), and queued reviews are already superseded at dequeue; this adds the mid-session window. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a pre-publish staleness gate for ReviewPullRequest tasks so the worker withholds review findings if the PR head SHA changes while a session is running, preventing publication of output against a commit the PR no longer targets.
Changes:
- Re-fetch PR refs immediately before publication and, if the head SHA changed, treat the run as superseded and complete the Check Run as
neutralwith a “Stale” output. - Add an additive
TaskListStatus::Supersededstate and wire it into thestatuscommand labeling. - Add a Wiremock-backed integration test covering the “head moved mid-session” withholding behavior; update README capability status.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| README.md | Updates capability status to reflect implemented stale-ref revalidation behavior for Check Runs. |
| crates/worker/src/lib.rs | Adds the stale-ref pre-publish revalidation gate, superseded publication path, and a Wiremock test ensuring findings are withheld when the PR head moves. |
| crates/webhook/src/routes.rs | Adds superseded labeling for the status command output. |
| crates/github/src/tasks.rs | Introduces TaskListStatus::Superseded and a mark_superseded helper on TaskStore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8 — the last remaining gap: pre-publish stale-ref revalidation.
What was already true on
mainThe target-resolution work that landed with #10 (
resolve_targets) had already satisfied most of #8's acceptance criteria:"HEAD"— issue tasks resolve the default-branch tip SHA, PR tasks resolve the PR's own head/base refs, all live from the GitHub APIcrates/webhook/tests/parse_fixtures.rs)Push/commit execution remains a contract-v3 lane (documented planned lane, tracked under #10's notes) — parsed and typed today.
What this PR adds
The one window still open: a PR head that moves while the review session runs. The supersession registry only guards dequeue; a review already in flight would still publish findings computed against a commit the PR no longer points at.
Now, after the session and before any publication,
ReviewPullRequesttasks re-fetch the PR refs and compare against the reviewed head:neutral/ "Stale" naming both SHAs, the marker-backed status comment flips toStatus: supersededwith retry guidance, and the Cave task list gets a new honest terminal statesuperseded(additiveTaskListStatusvariant, wired through thestatuscommand's labels)Verification
review_of_moved_head_is_published_as_superseded: first PR fetch returnssha-reviewed, the pre-publish re-fetch returnssha-moved; asserts neutral/Stale terminal check PATCH (neversuccess),Status: supersededon the comment surface, the finding text withheld, andTaskListStatus::Supersededin the storecargo check --all-targets,cargo clippy --all-targets -- -D warnings,cargo test --all(135 passed), python gates — all green